-
Notifications
You must be signed in to change notification settings - Fork 299
WIN-8498: Refactor FLRP Transaction Builders for Better Code Organization #7839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
542a8d5 to
0fda5db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the FLRP transaction builders to consolidate common properties (context, feeState, amount, utxos) into base classes, reducing code duplication. The refactoring also introduces standardized validation using BuildTransactionError and updates transaction building to use new FlareJS API methods (pvm.e.newImportTx, pvm.e.newExportTx, evm.newImportTx, evm.newExportTxFromBaseFee).
Key changes:
- Moved common setters to
TransactionBuilderandAtomicTransactionBuilderbase classes - Changed UTXO handling from
DecodedUtxoObj[]to hex string arrays with FlareJS native parsing - Introduced
ContextandFlrpFeeStatefor dynamic fee calculation - Updated all builders to use FlareJS helper methods instead of manual transaction construction
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| transaction.ts | Added _context, _feeState, _amount public properties; changed fee type to FlrpTransactionFee |
| transactionBuilder.ts | Moved context() setter and updated utxos() to accept hex strings; refactored validation |
| atomicTransactionBuilder.ts | Added feeState() and amount() setters; removed createInputOutput() method |
| ImportInPTxBuilder.ts | Refactored to use pvm.e.newImportTx; removed manual transaction construction |
| ImportInCTxBuilder.ts | Refactored to use evm.newImportTx; simplified fee calculation |
| ExportInPTxBuilder.ts | Refactored to use pvm.e.newExportTx; removed manual UTXO selection logic |
| ExportInCTxBuilder.ts | Refactored to use evm.newExportTxFromBaseFee; simplified construction |
| utils.ts | Added parseUtxoHex(), parseUtxoHexArray(), utxoToDecoded() for UTXO parsing |
| iface.ts | Added Context, FlrpTransactionFee, FeeConfig, ExportEVMOptions interfaces |
| networks.ts | Added flarePublicUrl property; changed txFee from 1261000 to 200000 |
| package.json | Added @bitgo/public-types@5.59.0 dependency |
| Test files | Updated test data with new transaction formats, context, and feeState |
Comments suppressed due to low confidence (1)
modules/sdk-coin-flrp/src/lib/utils.ts:231
- The parameter 'Output' has type 'any', but its name coincides with the imported type Output.
mapOutputToEntry(network: FlareNetwork): (Output) => Entry {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e66b7fa to
511051d
Compare
WIN-8498: Refactor FLRP Transaction Builders for Better Code Organization
This PR refactors the FLRP transaction builders to consolidate common properties and methods into base classes, reducing code duplication and improving maintainability.
🔄 Changes Overview
1. Consolidated Common Properties to Base Classes
context()TransactionBuildertransaction._contextfeeState()ExportInPTxBuilder,ImportInPTxBuilderAtomicTransactionBuildertransaction._feeStateamount()ExportInPTxBuilder,ExportInCTxBuilderAtomicTransactionBuildertransaction._amountutxos()ExportInPTxBuilder,ImportInPTxBuilderTransactionBuilder2. Added Validation for UTXO Balance
ExportInPTxBuilder: Validates thattotalUtxoAmount >= amountbefore buildingImportInCTxBuilder: Validates thattotalUtxoAmount > feeto ensure import can pay feesImportInPTxBuilder: Validates thattotalUtxoAmount > 03. Standardized Error Types
All validation errors now consistently use
BuildTransactionErrorinstead of genericError.📁 Files Modified
transaction.ts_feeState: FlrpFeeStateand_amount: bigintpublic propertiesatomicTransactionBuilder.tsfeeState()andamount()setter methodsExportInPTxBuilder.ts_amount,_feeState,amount(),feeState(),utxos()- now uses base classExportInCTxBuilder.ts_amount,_context,amount(),context()- now uses base class; fixedutxos()method signatureImportInPTxBuilder.ts_feeState,_context,feeState(),context(),utxos()- now uses base classImportInCTxBuilder.ts_context,context()- now uses base class; added UTXO balance validation✅ Benefits
context(),feeState(), andamount()are now in base classesBuildTransactionErrorfor validation failures🧪 Testing
All existing unit tests pass. The refactoring maintains backward compatibility - the public API remains unchanged.